chore: fix db selector read/writes #685
Conversation
WalkthroughIntroduces helpers to wrap raw stored selector maps into API JsonSelector objects on read and to unwrap JsonSelector objects back to raw maps on write; applies this pattern across deployments, environments, and policies. CEL selector paths are noted as not yet supported and replaced with TODOs in tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DBLayer
participant Storage
rect rgb(210,230,240)
Note over Client,DBLayer: Read flow — wrap stored map → JsonSelector
Client->>DBLayer: GetResources()/GetEnvironments()/GetPolicies()
DBLayer->>Storage: SELECT ... (returns raw map)
Storage-->>DBLayer: raw selector map
DBLayer->>DBLayer: wrapSelectorFromDB(raw map)
DBLayer-->>Client: object with JsonSelector
end
rect rgb(240,230,210)
Note over Client,DBLayer: Write flow — unwrap JsonSelector → store raw map
Client->>DBLayer: Create/Update resource with JsonSelector
DBLayer->>DBLayer: unwrapSelectorForDB(JsonSelector)
DBLayer->>Storage: INSERT/UPDATE with raw map
Storage-->>DBLayer: OK
DBLayer-->>Client: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)apps/workspace-engine/**/*.go📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Files:
🧬 Code graph analysis (2)apps/workspace-engine/pkg/db/common.go (2)
apps/workspace-engine/pkg/db/policies.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/workspace-engine/pkg/db/common.go(2 hunks)apps/workspace-engine/pkg/db/deployments.go(3 hunks)apps/workspace-engine/pkg/db/deployments_test.go(3 hunks)apps/workspace-engine/pkg/db/environments.go(2 hunks)apps/workspace-engine/pkg/db/environments_test.go(5 hunks)apps/workspace-engine/pkg/db/policies.go(2 hunks)apps/workspace-engine/pkg/db/policies_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/db/policies.goapps/workspace-engine/pkg/db/deployments_test.goapps/workspace-engine/pkg/db/common.goapps/workspace-engine/pkg/db/environments.goapps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/deployments.goapps/workspace-engine/pkg/db/environments_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/db/deployments_test.goapps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/environments_test.go
🧠 Learnings (4)
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/environments_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/environments_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Applied to files:
apps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/environments_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/db/policies_test.goapps/workspace-engine/pkg/db/environments_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/db/deployments_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
JsonSelector(191-193)
apps/workspace-engine/pkg/db/common.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Selector(363-365)JsonSelector(191-193)apps/workspace-engine/pkg/selector/resources.go (1)
Selector(11-11)
apps/workspace-engine/pkg/db/environments_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
JsonSelector(191-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
| // Get the raw unwrapped selector data from the database | ||
| var rawMap map[string]interface{} | ||
| selectorBytes, err := selector.MarshalJSON() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := json.Unmarshal(selectorBytes, &rawMap); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // It's an unwrapped ResourceCondition from the database, wrap it in JsonSelector format | ||
| wrappedSelector := oapi.JsonSelector{ | ||
| Json: rawMap, | ||
| } | ||
|
|
||
| return selector.FromJsonSelector(wrappedSelector) | ||
| } |
There was a problem hiding this comment.
Avoid double-wrapping selectors already stored in { "json": { ... } }.
Many existing rows still persist selectors in wrapped form ({"json":{...}}). After unmarshalling, this code blindly wraps that map again, producing {"json":{"json":{...}}}. Downstream callers (including AsJsonSelector) will then see an extra json envelope and fail to locate fields like type/operator. Existing deployments/environments/policies will break on read because their selectors no longer deserialize as expected. Please strip a single leading json key before calling FromJsonSelector, or otherwise detect already-wrapped data to preserve backwards compatibility.
Apply this diff:
- if err := json.Unmarshal(selectorBytes, &rawMap); err != nil {
- return err
- }
-
- // It's an unwrapped ResourceCondition from the database, wrap it in JsonSelector format
- wrappedSelector := oapi.JsonSelector{
- Json: rawMap,
- }
+ if err := json.Unmarshal(selectorBytes, &rawMap); err != nil {
+ return err
+ }
+
+ // Handle legacy rows that were already stored with a top-level "json" envelope.
+ if len(rawMap) == 1 {
+ if inner, ok := rawMap["json"]; ok {
+ switch typed := inner.(type) {
+ case map[string]interface{}:
+ rawMap = typed
+ case nil:
+ rawMap = nil
+ }
+ }
+ }
+
+ // Wrap the unwrapped ResourceCondition from the database in JsonSelector format.
+ wrappedSelector := oapi.JsonSelector{
+ Json: rawMap,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the raw unwrapped selector data from the database | |
| var rawMap map[string]interface{} | |
| selectorBytes, err := selector.MarshalJSON() | |
| if err != nil { | |
| return err | |
| } | |
| if err := json.Unmarshal(selectorBytes, &rawMap); err != nil { | |
| return err | |
| } | |
| // It's an unwrapped ResourceCondition from the database, wrap it in JsonSelector format | |
| wrappedSelector := oapi.JsonSelector{ | |
| Json: rawMap, | |
| } | |
| return selector.FromJsonSelector(wrappedSelector) | |
| } | |
| // Get the raw unwrapped selector data from the database | |
| var rawMap map[string]interface{} | |
| selectorBytes, err := selector.MarshalJSON() | |
| if err != nil { | |
| return err | |
| } | |
| if err := json.Unmarshal(selectorBytes, &rawMap); err != nil { | |
| return err | |
| } | |
| // Handle legacy rows that were already stored with a top-level "json" envelope. | |
| if len(rawMap) == 1 { | |
| if inner, ok := rawMap["json"]; ok { | |
| switch typed := inner.(type) { | |
| case map[string]interface{}: | |
| rawMap = typed | |
| case nil: | |
| rawMap = nil | |
| } | |
| } | |
| } | |
| // Wrap the unwrapped ResourceCondition from the database in JsonSelector format. | |
| wrappedSelector := oapi.JsonSelector{ | |
| Json: rawMap, | |
| } | |
| return selector.FromJsonSelector(wrappedSelector) | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/common.go around lines 30 to 47, the code
currently always wraps the unmarshalled map into oapi.JsonSelector, causing
double-wrapped selectors when the stored value is already in the form {"json":
{...}}; change the logic to detect and unwrap one leading "json" envelope: after
json.Unmarshal into rawMap, if rawMap has a top-level key "json" whose value is
itself a map[string]interface{}, set wrappedSelector.Json to that inner map
instead of rawMap, otherwise set wrappedSelector.Json = rawMap; then call
selector.FromJsonSelector with the normalized wrappedSelector so existing
wrapped rows are read compatibly.
There was a problem hiding this comment.
this is intentional, we should never store as json wrapped and vice versa.
There was a problem hiding this comment.
@adityachoudhari26, understood! I've noted that the database storage format is standardized to always store selectors in unwrapped format, while the application layer works with wrapped JsonSelector format. This design creates a clear contract at the storage boundary.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#685
File: apps/workspace-engine/pkg/db/common.go:0-0
Timestamp: 2025-10-16T20:59:57.054Z
Learning: In the ctrlplane workspace-engine, selectors must always be stored in the database in unwrapped format (raw condition tree without the "json" wrapper). The application code wraps them into JsonSelector format on read via `wrapSelectorFromDB()` and unwraps them for storage via `unwrapSelectorForDB()`. Never store selectors in wrapped `{"json": {...}}` format in the database.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Try as JsonSelector | ||
| jsonSelector, err := selector.AsJsonSelector() | ||
| if err == nil && jsonSelector.Json != nil { | ||
| // Return the unwrapped map directly - pgx can handle it | ||
| return jsonSelector.Json, nil | ||
| } | ||
|
|
||
| // CEL selectors are not supported - return nil to store NULL in database | ||
| // TODO: Add support for CEL selectors in the future | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Do not silently drop non-JSON selectors on write.
If a selector is backed by CEL (or any non-JSON variant), AsJsonSelector fails and this helper returns nil, causing writeEnvironment / writeDeployment / writeManySelectors to persist NULL. Any update will therefore delete existing CEL selectors from the database. Instead of discarding them, either preserve the raw bytes (e.g., marshal and store the existing representation) or fail fast so callers know the selector type is unsupported. The current behaviour causes immediate data loss for CEL selectors.
- jsonSelector, err := selector.AsJsonSelector()
- if err == nil && jsonSelector.Json != nil {
- // Return the unwrapped map directly - pgx can handle it
- return jsonSelector.Json, nil
- }
-
- // CEL selectors are not supported - return nil to store NULL in database
- // TODO: Add support for CEL selectors in the future
- return nil, nil
+ jsonSelector, err := selector.AsJsonSelector()
+ if err == nil && jsonSelector.Json != nil {
+ // Return the unwrapped map directly - pgx can handle it
+ return jsonSelector.Json, nil
+ }
+
+ // Fallback: preserve the raw union payload so non-JSON selectors are not lost.
+ rawBytes, err := selector.MarshalJSON()
+ if err != nil {
+ return nil, err
+ }
+ if len(rawBytes) == 0 || string(rawBytes) == "null" {
+ return nil, nil
+ }
+
+ var rawMap map[string]interface{}
+ if err := json.Unmarshal(rawBytes, &rawMap); err != nil {
+ return nil, err
+ }
+ return rawMap, nilCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/common.go around lines 57 to 67, the helper
currently swallows non-JSON selectors and returns nil which causes CEL (or other
non-JSON) selectors to be persisted as NULL and silently lost; instead, change
the function to not return nil for unsupported selectors: either (1) attempt to
preserve the selector by obtaining its raw representation (marshal or get raw
bytes of the selector) and return that for storage, or (2) if a raw
representation is not available, return a non-nil error so callers fail fast and
can handle unsupported selector types. Update the function to try Json first,
then try to extract raw bytes and return them if present, and only return an
error when no representation can be obtained; ensure callers of this helper
handle the returned error appropriately.
There was a problem hiding this comment.
this is intentional
There was a problem hiding this comment.
@adityachoudhari26, understood! I've noted that the current behavior of writing CEL selectors as NULL is intentional for this PR.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#685
File: apps/workspace-engine/pkg/db/common.go:47-57
Timestamp: 2025-10-16T20:59:56.422Z
Learning: In the ctrlplane workspace-engine db package, CEL selectors are intentionally not supported and are written as NULL to the database. This is documented with TODO comments for future implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Refactor
Tests